Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

geolocation button closes issue#780 #800

Merged
merged 42 commits into from
Apr 5, 2023
Merged

Conversation

Jacky0299
Copy link
Contributor

closes #780

AliyanH

This comment was marked as resolved.

src/mapml-viewer.js Outdated Show resolved Hide resolved
@AliyanH

This comment was marked as resolved.

Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to add the locate API method and associated events, to <mapml-viewer> and <map is=web-map>

Gruntfile.js Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
src/web-map.js Outdated Show resolved Hide resolved
@prushforth
Copy link
Member

prushforth commented Mar 27, 2023

The control has 3 states: off (black) tracking (orange) and last known location (blue). These visual states need to be available to screen readers, so that when the focus lands on the control, it reads out "Show my location control - location tracking mode" (for example). "Show my location - last known location mode" and "Show my location - location off".

We need to figure out how to convey the semantics of features' positions relative to your location, if shown, to screen reader users. I am thinking the as-the-crow-flies distance might be a good start. We don't have to to this last idea in this PR, but we definitely need to convey the location button states per my first comment.

@prushforth
Copy link
Member

@Jacky0299 you're working on this, is that correct?

The control has 3 states: off (black) tracking (orange) and last known location (blue). These visual states need to be available to screen readers, so that when the focus lands on the control, it reads out "Show my location control - location tracking mode" (for example). "Show my location - last known location mode" and "Show my location - location off".

@Jacky0299 Jacky0299 closed this Mar 28, 2023
@Jacky0299 Jacky0299 reopened this Mar 28, 2023
@Jacky0299
Copy link
Contributor Author

@Jacky0299 you're working on this, is that correct?

The control has 3 states: off (black) tracking (orange) and last known location (blue). These visual states need to be available to screen readers, so that when the focus lands on the control, it reads out "Show my location control - location tracking mode" (for example). "Show my location - last known location mode" and "Show my location - location off".

yes

@prushforth
Copy link
Member

@Jacky0299 L.Control.Locate is ending up in dist folder. It should be bundled via rollup with in leaflet.js. Checkout how the other controls manage this, use them as example of how-to.

@prushforth
Copy link
Member

@Jacky0299 is there any way to instead of the screen reader saying "Button" when you focus your location on the map, to have it say "Your (last known) / (current) location shown on the map", where "last known" or "curren" is said based on the tracking mode of the control? Since we've got it as a tab stop, we need to be more informative about what it is besides just "Button".

src/mapml-viewer.js Outdated Show resolved Hide resolved
src/mapml-viewer.js Outdated Show resolved Hide resolved
@@ -704,6 +704,9 @@ export class MapViewer extends HTMLElement {
}
locate(options){
if (this._map) {
if (this._geolocationButton) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jacky0299 Please add a test for the location API bug Fix. Thanks!

Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work Jacky! Looks good to me LGTM!

@prushforth prushforth requested a review from AliyanH April 5, 2023 18:25
Copy link
Member

@AliyanH AliyanH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great @Jacky0299, great work!

@Jacky0299 Jacky0299 merged commit d24a606 into Maps4HTML:main Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement geolocation control
3 participants